Code & Security Review

Summary

Clean, well-scoped feature addition. Forward compatibility is handled correctly, tests cover the key cases, and the dual-filter architecture (IssueTracker for notifications, AppState.filteredReviewRequests for reactive UI) is a sensible design. No security concerns.

Security Review

Strengths:

• Exclude list is a pure comparison key — no shell/SQL/query interpolation, no injection surface.
• Case-insensitive matching is applied consistently on both sides ($0.repo.lowercased() vs excludeSet of .lowercased() entries).
• Input sanitization in SettingsView is reasonable: trimmingCharacters(in: .whitespaces) + filter { !$0.isEmpty } prevents empty/whitespace entries from ever hitting the filter.
• No new privileged actions, scopes, or network calls — just a post-fetch filter.

Concerns: None.

Code Quality

Strengths:

• Custom init(from:) decoder on ConfigDefaults correctly uses decodeIfPresent for forward compat (matches the pattern already used on AppConfig). configDefaultsDecodeWithoutExcludeReviewRepos locks it in.
• IssueTracker.refresh reloads config from disk each poll via ConfigStore.loadConfig, so the server-side filter is always fresh — no stale state bug.
• AppState.excludeReviewRepos is written on both initial hydrate (AppDelegate.swift:117) and on every saveSettings (AppDelegate.swift:452), so the UI reacts immediately when the user updates settings.
• @Observable ensures the computed filteredReviewRequests / unseenReviewCount re-render on changes to either reviewRequests or excludeReviewRepos.

Minor observations (non-blocking):

🟡 Re-notification after toggle (IssueTracker.swift:258-264): The exclude filter runs before previousReviewRequestIDs is updated. Flow:

1. Exclude repo A → next poll drops A from reviews → previousReviewRequestIDs loses A's IDs.
2. Un-exclude repo A → next poll re-includes A → A's IDs now appear "new" → notification fires for PRs the user had already seen.

Not security-critical and arguably a rare workflow, but may confuse users who toggle the list. Could be fixed by computing newIDs against the unfiltered set before filtering, or by persisting previousReviewRequestIDs across filter changes. Worth a follow-up issue rather than a blocker.

🟡 Save-on-Enter only (SettingsView.swift:164-170): The .onSubmit modifier means users who type a repo and then click somewhere else (or close the window) will lose their edit silently. This matches the existing branchPrefix pattern on line 144, so it's consistent — but adding .onChange or a .onDisappear save would be more forgiving. Consistency vote probably wins here.

🟢 No format validation: "notarepo" (missing org/) is accepted but will never match any $0.repo (which is always org/repo from GraphQL). Silently ineffective rather than broken. A Text(...) caption below the field already documents the expected format, so this is fine.

🟢 Dual filter redundancy: Once the IssueTracker filter runs, appState.reviewRequests is already filtered, so filteredReviewRequests becomes a no-op on the next poll. This is intentional (per the PR description) to provide immediate UI feedback in the ~60s window before the next poll. Worth a brief comment on filteredReviewRequests noting the dual-layer rationale, but not necessary.

Tests

• appConfigRoundTrip updated to include the new field — good.
• configDefaultsDecodeWithoutExcludeReviewRepos explicitly locks in forward compat for configs predating this change — excellent.
• appConfigDecodeFromEmptyJSON asserts the default empty list — good.
• 113/113 tests pass locally.

Summary Table

Recommendation: Approve. The feature is well-implemented, tests are solid, forward compat is handled, and there are no blocking or security issues. The yellow items are follow-up polish, not merge-blockers.